ROX-33614: Update Falco to 0.23.1#2976
Open
Stringy wants to merge 4 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2976 +/- ##
==========================================
+ Coverage 27.34% 27.41% +0.06%
==========================================
Files 95 94 -1
Lines 5420 5410 -10
Branches 2545 2543 -2
==========================================
+ Hits 1482 1483 +1
+ Misses 3211 3200 -11
Partials 727 727
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
5 times, most recently
from
c2385e2 to
e647cac
Compare
Stringy
changed the title
ovalenti
changed the title
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
70ea6aa to
cd3770b
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR replaces stored container metadata lookups with cgroup-based ChangesContainer ID and metadata refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessSignalFormatter
participant ThreadManager
participant GetContainerID
participant ExtractContainerIDFromCgroup
ProcessSignalFormatter->>ThreadManager: traverse_parent_state(*mt, visitor)
ThreadManager-->>ProcessSignalFormatter: parent threadinfo pt
ProcessSignalFormatter->>GetContainerID: GetContainerID(*pt)
GetContainerID->>ExtractContainerIDFromCgroup: parse tinfo.cgroups()
ExtractContainerIDFromCgroup-->>GetContainerID: container ID or empty
GetContainerID-->>ProcessSignalFormatter: container ID or lineage boundary
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
544901c to
23c2967
Compare
Molter73
reviewed
Jun 15, 2026
Molter73
left a comment
Molter73
left a comment
Collaborator
There was a problem hiding this comment.
I haven't gone through the changes under .claude or AGENTS.md, I think those probably should moved to their own PR.
Even without those there are plenty of comments, the falco side looks fine though.
|
|
||
| CLOG(DEBUG) << "Process (" << signal->container_id() << ": " << signal->pid() << "): " | ||
| << signal->name() << "[" << container_metadata_.GetNamespace(event) << "] " | ||
| << signal->name() |
Collaborator
There was a problem hiding this comment.
Suggested change
| << signal->name() | |
| << signal->name() << "[" << container_id << "] " |
Collaborator
There was a problem hiding this comment.
Actually, I'm an idiot, the container ID is clearly printed in the line above this one 🤦🏻
Can we go back to not having this here?
Collaborator
There was a problem hiding this comment.
Why do the timeouts in this test need to be bumped from 5 to 30 seconds?
- Update falcosecurity-libs from 0.18.1 to 0.23.1 - Fix BPF verifier failures on older kernels (4.18) - Fix clang-format lint in Utility.cpp - Skip fd-based execs (/dev/fd/N) in exepath fallback - Disable TOCTOU 64-bit progs for missing syscalls - Remove container plugin, use built-in container ID lookups - Add analyze-ci Claude skill - Add update-falco-libs Claude skill Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use event type constants instead of strncmp for send/recv detection - Revert extract() back to extract_single() (still available in pinned falco) - Change null filter_check guards to asserts (fatal on init failure) - Move default_formatter_ back to initializer list - Remove try/catch around container filter (let exception propagate) - Use GetContainerID helper in Process.cpp instead of inline cgroup loop - Restore container ID in operator<< and debug log output
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
23c2967 to
6d57248
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
collector/lib/ContainerMetadata.cpp (1)
10-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
EventExtractorsetup fromContainerMetadata
GetNamespace()andGetContainerLabel()no longer useevent_extractor_orinspector_, so the constructor’sEventExtractor::Init()call is dead work. Remove the cached members and simplify the constructor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@collector/lib/ContainerMetadata.cpp` around lines 10 - 28, ContainerMetadata still initializes and stores EventExtractor/inspector even though GetNamespace() and GetContainerLabel() no longer use them, so remove the unused member setup. Simplify the ContainerMetadata::ContainerMetadata constructor by dropping the EventExtractor::Init(inspector) call and any related cached state, and update the class to only keep the symbols actually used by GetNamespace(const std::string&) and GetContainerLabel().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@collector/lib/ContainerMetadata.cpp`:
- Around line 10-28: ContainerMetadata still initializes and stores
EventExtractor/inspector even though GetNamespace() and GetContainerLabel() no
longer use them, so remove the unused member setup. Simplify the
ContainerMetadata::ContainerMetadata constructor by dropping the
EventExtractor::Init(inspector) call and any related cached state, and update
the class to only keep the symbols actually used by GetNamespace(const
std::string&) and GetContainerLabel().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2f9927b3-1b5c-4b33-bdf1-1bb0f2447a31
📒 Files selected for processing (22)
collector/CMakeLists.txtcollector/lib/CollectorService.cppcollector/lib/ContainerEngine.hcollector/lib/ContainerInfoInspector.cppcollector/lib/ContainerInfoInspector.hcollector/lib/ContainerMetadata.cppcollector/lib/NetworkSignalHandler.cppcollector/lib/Process.cppcollector/lib/ProcessSignalFormatter.cppcollector/lib/ProcessSignalFormatter.hcollector/lib/Utility.cppcollector/lib/Utility.hcollector/lib/system-inspector/EventExtractor.cppcollector/lib/system-inspector/EventExtractor.hcollector/lib/system-inspector/Service.cppcollector/lib/system-inspector/Service.hcollector/test/ProcessSignalFormatterTest.cppcollector/test/SystemInspectorServiceTest.cppcollector/test/UtilityTest.cppfalcosecurity-libsintegration-tests/suites/k8s/namespace.gointegration-tests/suites/udp_networkflow.go
💤 Files with no reviewable changes (3)
- collector/lib/ContainerEngine.h
- collector/lib/system-inspector/Service.h
- collector/lib/ContainerInfoInspector.h
The sinsp filter parser treats the RHS of '!=' as a bare string value, not a field reference. Using val(proc.vpid) tells the parser to resolve proc.vpid as a field extraction at runtime, enabling the field-to-field comparison needed to filter host processes. Also fix clang-format alignment on the FIELD_CSTR macro.
Molter73
reviewed
Jul 14, 2026
Molter73
left a comment
Molter73
left a comment
Collaborator
There was a problem hiding this comment.
Overall looks good, I would like to get confirmation on the sinsp header removal comment and I believe there is still one comment from my previous review that has not been addressed.
Comment on lines
+8
to
+10
| if (!check) { | ||
| CLOG(FATAL) << "Filter check not available for field: " << wrapper->event_name; | ||
| } |
Collaborator
There was a problem hiding this comment.
[nitpick]
Suggested change
| if (!check) { | |
| CLOG(FATAL) << "Filter check not available for field: " << wrapper->event_name; | |
| } | |
| CLOG_IF(!check, FATAL) << "Filter check not available for field: " << wrapper->event_name; |
Comment on lines
+65
to
+68
| auto factory = std::make_shared<sinsp_filter_factory>( | ||
| inspector_.get(), EventExtractor::FilterList()); | ||
| sinsp_filter_compiler compiler(factory, "proc.pid != val(proc.vpid)"); | ||
| inspector_->set_filter(compiler.compile(), "proc.pid != val(proc.vpid)"); |
Collaborator
There was a problem hiding this comment.
This should be equivalent:
Suggested change
| auto factory = std::make_shared<sinsp_filter_factory>( | |
| inspector_.get(), EventExtractor::FilterList()); | |
| sinsp_filter_compiler compiler(factory, "proc.pid != val(proc.vpid)"); | |
| inspector_->set_filter(compiler.compile(), "proc.pid != val(proc.vpid)"); | |
| inspector_->set_filter("proc.pid != val(proc.vpid)"); |
Comment on lines
+61
to
+63
| // Filter out host processes. In containers, pid != vpid due to PID | ||
| // namespacing. This is a built-in sinsp field that doesn't require | ||
| // any plugin. |
Collaborator
There was a problem hiding this comment.
I think this may still need addressing, was a manual test at least run to verify if the behavior is consistent?
Comment on lines
+84
to
+94
| auto type = evt->get_type(); | ||
| bool is_send_recv = (type >= PPME_SOCKET_SENDTO_E && type <= PPME_SOCKET_RECVFROM_X) || | ||
| (type >= PPME_SOCKET_SENDMSG_E && type <= PPME_SOCKET_RECVMMSG_X); | ||
| if (!is_send_recv) { | ||
| if (fd_info->is_socket_failed()) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| if (fd_info->is_socket_pending()) { | ||
| return std::nullopt; | ||
| } |
Collaborator
There was a problem hiding this comment.
Might want to do something closer to what falco does:
Suggested change
| auto type = evt->get_type(); | |
| bool is_send_recv = (type >= PPME_SOCKET_SENDTO_E && type <= PPME_SOCKET_RECVFROM_X) || | |
| (type >= PPME_SOCKET_SENDMSG_E && type <= PPME_SOCKET_RECVMMSG_X); | |
| if (!is_send_recv) { | |
| if (fd_info->is_socket_failed()) { | |
| return std::nullopt; | |
| } | |
| if (fd_info->is_socket_pending()) { | |
| return std::nullopt; | |
| } | |
| switch (evt->get_type()) { | |
| case PPME_SOCKET_SENDTO_E: | |
| case PPME_SOCKET_SENDTO_X: | |
| case PPME_SOCKET_RECV_E: | |
| case PPME_SOCKET_RECV_X: | |
| case PPME_SOCKET_RECVFROM_E: | |
| case PPME_SOCKET_RECVFROM_X: | |
| case PPME_SOCKET_SENDMSG_E: | |
| case PPME_SOCKET_SENDMSG_X: | |
| case PPME_SOCKET_SENDMMSG_E: | |
| case PPME_SOCKET_SENDMMSG_X: | |
| case PPME_SOCKET_RECVMSG_E: | |
| case PPME_SOCKET_RECVMSG_X: | |
| case PPME_SOCKET_RECVMMSG_E: | |
| case PPME_SOCKET_RECVMMSG_X: | |
| if (fd_info->is_socket_failed()) { | |
| return std::nullopt; | |
| } | |
| if (fd_info->is_socket_pending()) { | |
| return std::nullopt; | |
| } | |
| break; | |
| default: | |
| break; |
It's more verbose, but also more explicit about what operations should make these checks. The compiler should still be able to optimize the case conditions to range checks.
|
|
||
| #include <uuid/uuid.h> | ||
|
|
||
| #include <libsinsp/sinsp.h> |
Collaborator
There was a problem hiding this comment.
Oh lord, no, please let's not start polluting the code base with sinsp headers again, we even have a proposal to stop doing this (#3502).
AFAICT, this is only needed for the sinsp_thread_manager definition, can we use a forward definition in the header file like we do with sinsp and sinsp_threadinfo?
Collaborator
There was a problem hiding this comment.
Actually, I tried locally removing it and it just compiled, so maybe it is not needed at all.
|
|
||
| CLOG(DEBUG) << "Process (" << signal->container_id() << ": " << signal->pid() << "): " | ||
| << signal->name() << "[" << container_metadata_.GetNamespace(event) << "] " | ||
| << signal->name() |
Collaborator
There was a problem hiding this comment.
Actually, I'm an idiot, the container ID is clearly printed in the line above this one 🤦🏻
Can we go back to not having this here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Falco PR: stackrox/falcosecurity-libs#97
The changes in this PR relate to the uplift of Falco to the latest tagged version, 0.23.1. This is a significant upgrade, jumping from 0.18.1 up to this latest version and contains the following notable changes to Collector:
This work was performed primarily by Claude, with oversight from me. This was something of an experiment and so I got Claude to do as much of the work as possible, with my role being that of the driver; steering Claude when it got muddled or hyper focused on the wrong fix.
The update and rebase itself was relatively straight forward, resulting in new locally-built and locally-verified builds in just a couple of hours (i.e. builds that built and ran locally, passing the integration tests on my Fedora 42 x86 machine.) The bottle neck became the CI and getting that work fed back into Claude to perform the diagnostics and fixes.
As a result, I have added two Claude skills as part of this PR which allow Claude to (1) perform Falco updates in the future and (2) inspect the state of CI and investigate test failures or BPF verifiers issues.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Built and tested locally, all unit & integration tests passing. CI handles the remainder of our test matrix.